Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implementation of rlp::decode() #405

Merged
merged 18 commits into from
Oct 18, 2023
Merged

Implementation of rlp::decode() #405

merged 18 commits into from
Oct 18, 2023

Conversation

Quentash
Copy link
Contributor

@Quentash Quentash commented Oct 5, 2023

Implementation of rlp::decode() as it was benchmarked and tested here (https://github.com/danilowhk/cairo-lib/blob/main/src/encoding/rlp.cairo).

Pull Request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build-related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Resolves: #

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

@Quentash
Copy link
Contributor Author

Quentash commented Oct 5, 2023

I clearly misunderstood the assignment that I have been given. I indeed thought that I was asked to implement the whole eth_transaction.cairo file by using the rlp.cairo fork linked above as an external import.
As far as I understand now, I was asked to import the rlp.cairo file into kakarot source files. Which I therefore quickly did.
I've left what I experimented in eth_transaction.cairo because it was using what I think to be a good example ( taken from here : https://eips.ethereum.org/EIPS/eip-155 ) and somehow also tests the rlp::decode() utils in a more "concrete" way.

I am terribly sorry if whatever I have done here is completly out of scope! I'm well aware some code will need to be removed but pushing now allow me to clarify this confusion with you guys so I can finish what I have been asked!

Eikix
Eikix reviewed Oct 6, 2023
View reviewed changes
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
@Eikix Eikix mentioned this pull request Oct 6, 2023
Closed
@Eikix
Copy link
Member

Eikix commented Oct 6, 2023

The scope of this PR is too large. Could you break it down into two PRs, one for the RLP trait & RLP decode
And the second one for the EOA logic: decode_tx?

Eikix
Eikix reviewed Oct 9, 2023
View reviewed changes
crates/utils/src/math.cairo Outdated Show resolved Hide resolved
Eikix
Eikix reviewed Oct 9, 2023
View reviewed changes
crates/utils/src/math.cairo Outdated Show resolved Hide resolved
Eikix
Eikix reviewed Oct 9, 2023
View reviewed changes
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
Eikix
Eikix reviewed Oct 9, 2023
View reviewed changes
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
Eikix
Eikix reviewed Oct 9, 2023
View reviewed changes
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
Eikix
Eikix reviewed Oct 9, 2023
View reviewed changes
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
Eikix
Eikix reviewed Oct 9, 2023
View reviewed changes
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
Eikix
Eikix reviewed Oct 9, 2023
View reviewed changes
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
Eikix
Eikix reviewed Oct 9, 2023
View reviewed changes
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
enitrat
enitrat reviewed Oct 12, 2023
View reviewed changes
crates/utils/src/helpers.cairo Outdated Show resolved Hide resolved
crates/utils/src/math.cairo Outdated Show resolved Hide resolved
crates/utils/src/math.cairo Outdated Show resolved Hide resolved
crates/utils/src/math.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/tests/test_rlp.cairo Outdated Show resolved Hide resolved
enitrat
enitrat reviewed Oct 16, 2023
View reviewed changes
Copy link
Collaborator

@enitrat enitrat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

almos lgtm now wdyt @Eikix

crates/utils/src/helpers.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
@Eikix
Copy link
Member

Eikix commented Oct 16, 2023

almos lgtm now wdyt @Eikix

agreed

@Quentash Quentash mentioned this pull request Oct 16, 2023
Closed
enitrat
enitrat reviewed Oct 18, 2023
View reviewed changes
Copy link
Collaborator

@enitrat enitrat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, proposed variable name changes to make it easier to understand on first read.

crates/utils/src/helpers.cairo Outdated Show resolved Hide resolved
crates/utils/src/helpers.cairo Outdated Show resolved Hide resolved
crates/utils/src/helpers.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
crates/utils/src/rlp.cairo Outdated Show resolved Hide resolved
enitrat
enitrat approved these changes Oct 18, 2023
View reviewed changes
Copy link
Collaborator

@enitrat enitrat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

image

@enitrat enitrat added this pull request to the merge queue Oct 18, 2023
Merged via the queue into kkrt-labs:main with commit 78cd4ab Oct 18, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eikix Eikix Eikix left review comments

@enitrat enitrat enitrat approved these changes

@ClementWalter ClementWalter Awaiting requested review from ClementWalter ClementWalter is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Quentash @Eikix @enitrat